# [fix](topn lazy materialization) Disable topn lazy materialization on non-light-schema-change tables#65415
Open
liutang123 wants to merge 2 commits into
Open
# [fix](topn lazy materialization) Disable topn lazy materialization on non-light-schema-change tables#65415liutang123 wants to merge 2 commits into
liutang123 wants to merge 2 commits into
Conversation
liutang123
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed PR 65415 for TopN lazy materialization gating on non-light-schema-change OLAP tables.
Critical checkpoint conclusions:
- Goal and coverage: The PR disables OLAP TopN lazy materialization before source selection when the table is AGG_KEYS or has light_schema_change=false, and adds a regression covering explicit false and true table properties.
- Scope: The change is small and focused on MaterializeProbeVisitor support checks plus one targeted regression suite.
- Concurrency/lifecycle: No new concurrency, locks, async lifecycle, static initialization, persistence, or transaction paths are introduced.
- Config/session compatibility: No new config is added. Existing topn_lazy_materialization_threshold and topn_lazy_materialization_using_index paths remain unchanged; the new table-property gate is applied in the normal scan, catalog-relation, and using-index filter probe paths.
- Parallel paths: The relevant OLAP probe entry points are covered; non-OLAP lazy materialization paths are unchanged.
- Test coverage: The regression checks both plan shape and query result for light_schema_change=false fallback and light_schema_change=true lazy behavior. I did not run the regression locally in this shallow Actions checkout because thirdparty/installed and thirdparty/installed/bin/protoc are missing; GitHub CI currently has style/license checks passing, with compile/FE UT/performance still pending.
- User focus: No additional user-provided focus points were supplied.
Subagent conclusions: optimizer-rewrite reported NO_NEW_VALUABLE_FINDINGS; tests-session-config reported NO_NEW_VALUABLE_FINDINGS. No subagent candidate became an inline comment, and none were dismissed or merged as duplicates. Final convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same empty inline-comment set and this comment-only review body.
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29611 ms |
Contributor
TPC-DS: Total hot run time: 181189 ms |
Contributor
ClickBench: Total hot run time: 25.09 s |
… non-light-schema-change tables TopN lazy materialization appends a synthetic global row-id column (__DORIS_GLOBAL_ROWID_COL__<table>) to the OLAP scan. The BE only rebuilds the tablet schema from FE's columns_desc when columns_desc[0].col_unique_id >= 0, which only holds for light_schema_change tables. On a non-light-schema-change table every column's uniqueId is -1, so the synthetic row-id column never enters the BE tablet schema and the scan fails with: field name is invalid. field=__DORIS_GLOBAL_ROWID_COL__<table> (forcing the column into the scan schema instead makes the second-phase fetch silently return NULL for the lazily-materialized columns, which is worse). Fix: disable topn lazy materialization for non-light-schema-change OLAP tables in MaterializeProbeVisitor and fall back to normal topn (the working two-phase read path). A new helper supportOlapTopnLazyMaterialize() consolidates the existing AGG_KEYS exclusion with the new light_schema_change requirement, applied at visitPhysicalOlapScan, visitPhysicalCatalogRelation and visitPhysicalFilter. Add regression test topn_lazy_light_schema_change verifying: - light_schema_change=false: no lazy materialization in the plan, correct results. - light_schema_change=true: lazy materialization still applies, correct results. Also stub OlapTable.getEnableLightSchemaChange() in MaterializeProbeVisitorTest's mock so the existing access-path unit tests exercise a lazy-materializable table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
liutang123
force-pushed
the
fix-topn-lazy-non-lsc-table
branch
from
39f90f2 to
f4d8b5e
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29708 ms |
Contributor
TPC-DS: Total hot run time: 180169 ms |
Contributor
ClickBench: Total hot run time: 25 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29711 ms |
Contributor
TPC-DS: Total hot run time: 179028 ms |
Contributor
ClickBench: Total hot run time: 25.11 s |
Contributor
Author
|
@morrySnow Hi, do you have time to see this BUG? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A top-N query that emits a non-order-by column fails on a non-light-schema-change OLAP table (a table created/upgraded with
light_schema_change = false, where every column'suniqueIdis-1):Fix: disable topn lazy materialization for non-light-schema-change OLAP tables in MaterializeProbeVisitor and fall back to normal topn (the working two-phase read path). A new helper supportOlapTopnLazyMaterialize() consolidates the existing AGG_KEYS exclusion with the new light_schema_change requirement, applied at visitPhysicalOlapScan, visitPhysicalCatalogRelation and visitPhysicalFilter.
Add regression test topn_lazy_light_schema_change verifying:
Behavior after the fix
light_schema_change = falsePhysicalOlapScan(no lazy) → safe two-phase readlight_schema_change = truePhysicalLazyMaterialize/PhysicalLazyMaterializeOlapScan(unchanged)What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)